Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support optional types for computed attributes #170

Merged
merged 2 commits into from Mar 8, 2013
Merged

Support optional types for computed attributes #170

merged 2 commits into from Mar 8, 2013

Conversation

MattRogish
Copy link
Contributor

We're using Serializer.schema to auto-generate our Ember.js schema. We added a method that returns a string, but AMS is sending 'nil' since it's computed, which messes up the schema. This PR allows you to specify an optional type for a computed attribute so that your schema matches what you expect.

@@ -73,7 +73,9 @@ def attributes(*attrs)
end

def attribute(attr, options={})
self._attributes = _attributes.merge(attr => options[:key] || attr.to_s.gsub(/\?$/, '').to_sym)
self._attributes = _attributes.merge(attr.is_a?(Hash) ? attr : {attr => options[:key] || attr.to_s.gsub(/\?$/, '').to_sym})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, that is a busy line if code.

@joliss
Copy link
Contributor

joliss commented Jan 3, 2013

I wonder if we should take back a step and think about if and how we want schema support to happen. Right now it feels a bit like we're heaping functionality onto a fairly obscure feature.

In general generated schemas seem to be great for DRYing up complex apps. In my experience, it doesn't scale well if you have to to keep server-side and client-side models in sync. AMS's schema method comes in handy to generate model definitions for your client-side framework. Matt and I have been using this, and it's quite convenient.

On the other hand, the way it's currently implemented, types are inferred from ActiveRecord, which requires an active DB connection. This means that you cannot use the schema in any asset files, since there's not necessarily a database around during precompilation. The alternative is generating a schema.js file using a rake task. That's what we've been doing, but it feels like a bit of a hack. For instance you have to remember to regenerate the schema. It's not something I'd recommend as a canonical solution.

I wonder if there's a way to make schemas work in the asset pipeline. Or maybe you guys have other ideas as to what to do with the schema?

@steveklabnik
Copy link
Contributor

I am very, very against this change. 👎. Types don't belong in JSON: schemas are an anti-pattern.

@MattRogish
Copy link
Contributor Author

Steve,

I generally agree - I don't want JSON to turn into a slightly easier to read XML + XSD.

But -- as Jo said, we're using the AMS schema to auto-generate a lot of stuff for our Ember.js front-end. Losing schema support would relegate us to keeping manual mappings, which feels very un-Rails and Ember-like. Better living through automation.

I struggle to think of another way we could automate this without, basically, us copypasta'ing this code into our Rails app, which we would certainly do should schema support disappear.

That said, given that schema support is a "supported" bit of AMS, why 👎 something that makes it a bit more useful?

Matt

@steveklabnik
Copy link
Contributor

Better living through automation.

I agree for sure. I need to better educate myself on the Ember side of things, which is why I wrote 👎 instead of closing. ;) Everything should automatically work; schemas aren't needed to make that happen properly.

@MattRogish
Copy link
Contributor Author

Steve,

Thanks for the reply and the discussion. I understand and I agree with you that the AMS schema isn't necessary for Ember to work.

Ember "out of the box" totally doesn't need AMS schema; you just define your Ember models by hand, which is not impossible obviously but is a major bummer and a easy way to introduce bugs and other unanticipated behavior. :)

Thus, we decided that autogenerating our Ember model definitions from the Serializer schema was a net win in what we were doing. In the general case of {JS front end} and {Rails/Sinatra back-end}, AMS acts as the interface between, and code generation should enter into it somewhere.

Absolutely the AMS schema isn't a necessary component for that either; it just happens to be a very useful (and existing!) bit that we piggy back on. If AMS schemas go away, we'd need to just relocate that code into a helper; presumably the Ember-Rails project would be a better place for this interface to live, anyway.

I'd certainly be in favor of removing the AMS schema and then relocating the model definition generation (if they are amenable to it) to the Ember-Rails project.

In the interim, this change would help us remove some bit of manual work (since they're returning nil now) and further get us closer to complete automation, but I understand if it isn't furthering the goals of the AMS project. If so, I'd think some sort of deprecation of schemas ought to take place so we can look for a better place to put this stuff, anyway.

Perhaps the step I can do is extract our automation and the schema generation into something digestible by the Ember-Rails project, and continue the discussion over there?

Ref (Ember models):
http://emberjs.com/guides/models/defining-models/

@steveklabnik
Copy link
Contributor

So.

Is this still useful? I need to investigate further into what the latest details are, but Ember has changed a lot in the last two months. Do we still want to get this in in some form?

@joliss
Copy link
Contributor

joliss commented Mar 6, 2013

Not sure about this PR, but Ember still has model definitions that need to stay in sync with the serializer, yes.

@steveklabnik
Copy link
Contributor

Do they need something like this to stay in sync? I really need to embed an ember app into AMS's test suite ;)

@joliss
Copy link
Contributor

joliss commented Mar 6, 2013

Yes, we basically have a fundamental problem here that either you have to manually keep Rails serializers and Ember models in sync, or you have to write (slightly hacky) custom code to generate models from the AMS schema.

@joliss
Copy link
Contributor

joliss commented Mar 6, 2013

I don't know how many people are actually using the AMS schema to generate Ember models. I imagine it's not super common.

@steveklabnik
Copy link
Contributor

Ahhh, that's the part that wasn't clicking for me: it's not about when the app already exists, it's about generating files. Right.

@joliss
Copy link
Contributor

joliss commented Mar 7, 2013

Ah, sorry I was unclear: The "generating" actually happens on a regular basis while you develop (semi-automatically) -- it's not just a helper for getting started. The purpose is to keep Ember models and Rails serializers DRY.

@MattRogish
Copy link
Contributor Author

I think Jo said most of it: we're using this to dynamically generate the Ember schema on a semi-continuous basis (when we change something in the serializer) - not just at the Ember app creation time.

My thinking is:
The current #schema implementation feels almost, but not quite, done. Dynamic methods returning nil for types seems incomplete; this change (I don't care if it's done via this PR, or some other PR) would at least bring closure to the schema generation process and ensure that no unexpected nils wind up in there.

That said, I'm also completely happy to ignore it if it doesn't fit in with the philosophy (although, why have Schemas in that case?), it doesn't cause any observable problems in the Ember app if we leave this out.

steveklabnik added a commit that referenced this pull request Mar 8, 2013
Support optional types for computed attributes
@steveklabnik steveklabnik merged commit e76a164 into rails-api:master Mar 8, 2013
@steveklabnik
Copy link
Contributor

It's all good, everyone. I'm still getting up to speed on some of the details of Ember, so thank you for putting up with my n00bness. :)

Anyway, obviously this is something that's needed, so let's merge it in. ❤️

@MattRogish
Copy link
Contributor Author

Thanks Steve! You rock!! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants